Skip to content

allow setting of secret description#134

Open
chippyash wants to merge 2 commits intozalando:masterfrom
chippyash:feature/set_description
Open

allow setting of secret description#134
chippyash wants to merge 2 commits intozalando:masterfrom
chippyash:feature/set_description

Conversation

@chippyash
Copy link

This allows developers to set the description of the secret being stored. Only useful for gnome-keyring based distributions but nevertheless important for users of those systems.

Signed-off-by: Ashley Kitson <ashley.kitson@zinc.systems>
}

func TestSetDescription(t *testing.T) {
if runtime.GOOS != "windows" && runtime.GOOS != "darwin" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO switch case would be more readable

type secretServiceProvider struct{}

// secretDescription holds a user set secret description
var secretDescription string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be a member of the secretServiceProvider

var secretDescription string

// SetDescription sets the description of the secret
func (s secretServiceProvider) SetDescription(description string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will likely need to be a pointer receiver, if we have it as member


// SetDescription sets the description of the secret
func (k windowsKeychain) SetDescription(description string) {
//no-op for windows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why should we have a comment here but not in the others?

Copy link
Member

@szuecs szuecs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR, from code pov I would say it needs some changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants